Skip to content

Popkov- Project Quest#12

Open
Coderdecoder07 wants to merge 18 commits intodemologin:mainfrom
Coderdecoder07:popkov
Open

Popkov- Project Quest#12
Coderdecoder07 wants to merge 18 commits intodemologin:mainfrom
Coderdecoder07:popkov

Conversation

@Coderdecoder07
Copy link

No description provided.

@demologin
Copy link
Owner

Общий вывод по проекту

Проект демонстрирует хорошее понимание основ сервлетов и архитектуры Command. Однако, стоит уделить больше внимания безопасности (валидация входных данных) и чистоте кода (избавление от магических строк и вложенной тернарной логики). Тесты покрывают основные сценарии, но местами опираются на хрупкие проверки строковых представлений.

Рекомендации:

  • Продолжить углубление знаний по паттернам проектирования
  • Изучить асинхронное программирование более глубоко
  • Уделить внимание документированию кода

Итоговая оценка: A

public String doPost(HttpServletRequest request) {
if (request.getParameter("logout") == null) {
HttpSession session = request.getSession();
User user = (User) session.getAttribute("user");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование 'магических строк' для атрибутов сессии. Рекомендуется использовать константы из класса Key для поддержания единообразия.

if (request.getParameter("logout") == null) {
HttpSession session = request.getSession();
User user = (User) session.getAttribute("user");
return Go.EDIT_USER + "?id=" + user.getId();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Отсутствует проверка на null для объекта user перед вызовом getId(), что может привести к NullPointerException.

@Override
public String doGet(HttpServletRequest req) {
req.setAttribute(QUESTS, questService.getAll());
return getView();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лишний пустой метод или дублирование логики базового интерфейса. Стоит проверить необходимость переопределения, если логика стандартна.


@Override
public String doGet(HttpServletRequest request) {
Long questId = Long.parseLong(request.getParameter(Key.QUEST_ID));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прямое парсинг параметра без проверки на null или формат числа. Может вызвать NumberFormatException.

} else {
String message = "Нет незавершенной игры";
log.warn(message);
RequestHelpers.createError(request, message);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дублирование логики создания ошибок. Рекомендуется вынести обработку исключительных ситуаций в глобальный фильтр или контроллер.

private void showOneQuestion(HttpServletRequest request, Game game) {
request.setAttribute(Key.GAME, game);
Optional<Question> question = questionService.get(game.getCurrentQuestionId());
request.setAttribute(Key.QUESTION, question.orElseThrow());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вызов orElseThrow() без указания конкретного исключения. Лучше выбрасывать кастомное исключение с понятным описанием.

public String doGet(HttpServletRequest req) {
String stringId = req.getParameter(Key.ID);
if (stringId != null) {
long id = Long.parseLong(stringId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Небезопасное приведение строки к long. Отсутствует обработка ошибок ввода (валидация ID).

String roleParam = req.getParameter("role");
String genderParam = req.getParameter("gender");
Role role = roleParam != null
? Role.valueOf(roleParam)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование тернарных операторов такой вложенности снижает читаемость. Рекомендуется использовать методы-хелперы.

}
String imageId = "image-" + user.getId();
Part imagePart = req.getPart("image");
if (imagePart != null && imagePart.getInputStream().available() > 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логика проверки наличия стрима через available() может быть ненадежной для всех типов Part. Лучше проверять размер или имя файла.


class ProfileIT extends BaseIT {

private final Profile profile = Winter.find(Profile.class);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Поле profile не инициализируется через Mockito или конструктор, а напрямую через Winter. Это усложняет изоляцию тестов.


String uri = signup.doPost(request);
Assertions.assertEquals(Go.PROFILE, uri);
Assertions.assertTrue(repository.getAll().toString().contains("newTestLogin"));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверка содержания строки через toString().contains() является хрупкой. Лучше проверять наличие объекта в коллекции через equals/id.

@Test
@DisplayName("When login admin then redirect to profile")
void whenLoginAdminThenRedirectToProfile() {
User user = userService.getAll().stream().findFirst().orElseThrow();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование stream().findFirst().orElseThrow() в тестах без предварительной подготовки данных может привести к нестабильности.

User admin = User.builder().id(1L).role(Role.ADMIN).build();
when(session.getAttribute(Key.USER)).thenReturn(admin);
when(request.getParameter(Key.NAME)).thenReturn("TestQuest");
when(request.getParameter(Key.TEXT)).thenReturn("1: Test OK?\n2< Да\n3< Нет\n2+ win\n3- lost\n");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хардкод тестовых данных (текст квеста) прямо в коде теста. Рекомендуется выносить такие блоки в ресурсы или константы.


class UserRepositoryTest {

private final UserRepository userRepository = new UserRepository();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Репозиторий инициализируется вручную через new, что противоречит общему стилю использования DI (Winter).

import com.javarush.popkov.util.Key;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.Part;
import lombok.SneakyThrows;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование @SneakyThrows скрывает проверяемые исключения, что затрудняет отладку и понимание логики обработки ошибок.


@SuppressWarnings("unused")
@Slf4j
public class PlayGame implements Command {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Класс помечен @slf4j, но логгирование используется точечно. Стоит добавить логирование критических переходов в игре.

import jakarta.servlet.http.HttpSession;

@SuppressWarnings("unused")
public class Profile implements Command {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Класс Command должен быть максимально тонким. Логика определения переходов (Go.EDIT_USER) размазана по командам.

protected User testUser;
protected User testGuest;

protected BaseIT() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Конструктор базового класса выполняет слишком много работы (инициализация моков, данных). Лучше использовать @beforeeach.

Mockito.when(request.getParameter(Key.QUESTION_ID)).thenReturn("1");
Mockito.when(request.getParameter(Key.TEXT)).thenReturn("newTestTextQuestion");
String actualUri = quest.doPost(request);
String expectedUri = "%s?id=%d#bookmark%d".formatted(Go.QUEST, 1, 1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сложное форматирование ожидаемого URI. Есть риск ошибки при изменении структуры ссылок.

import static com.javarush.popkov.util.Key.QUESTS;

@SuppressWarnings("unused")
@AllArgsConstructor
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аннотация @AllArgsConstructor из Lombok удобна, но в Java 21 для DTO лучше использовать Records (хотя здесь это Command).

? Gender.valueOf(genderParam)
: existingUser != null ? existingUser.getGender() : Gender.MALE;
User user = User.builder()
.login(req.getParameter("login"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Параметры 'login' и 'password' не проходят валидацию на пустые строки или длину.


@Override
public String doPost(HttpServletRequest request) {
Long gameId = RequestHelpers.getId(request);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование статического хелпера RequestHelpers затрудняет мокирование и тестирование логики команд.

when(request.getParameter("password")).thenReturn("err");

String actualRedirect = login.doPost(request);
Assertions.assertEquals("/login", actualRedirect);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверка только URI редиректа. Стоит также проверять отсутствие атрибута USER в сессии при ошибке.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants